The WhenMvnCliFailsWithNonAuthError_SetsFallbackReasonToOther_Async test does not assert that the FailedEndpoints telemetry key is absent when the Maven CLI failure is not due to authentication. The equivalent test in the now-deleted MavenWithFallbackDetectorTests.cs had this assertion: detectorResult.AdditionalTelemetryDetails.Should().NotContainKey("FailedEndpoints"). Without this assertion, if a bug caused FailedEndpoints to be set on non-auth errors, it would go undetected.

Several important test scenarios from the deleted MavenWithFallbackDetectorTests.cs are not ported to MvnCliDetectorTests.cs. These scenarios now apply to the promoted MvnCliComponentDetector but have no test coverage:

1. Transitive dependency preservation: WhenMavenCliSucceeds_PreservesTransitiveDependencies_Async — verifies that when Maven CLI succeeds, the dependency graph (parent → transitive) is preserved correctly.
2. Complete CLI failure with nested modules: WhenMvnCliFailsCompletely_AllNestedPomXmlsAreRestoredForStaticParsing_Async — verifies that when CLI fails for all pom.xml files, nested modules are restored for static parsing.
3. Partial CLI failure: WhenMvnCliPartiallyFails_NestedPomXmlsRestoredOnlyForFailedDirectories_Async — verifies that nested pom.xml files under directories that succeeded with CLI are not statically parsed, while directories where CLI failed are restored for static parsing.
4. DetectionMethod telemetry: No test verifies the DetectionMethod=MvnCliOnly telemetry entry when CLI succeeds for all files.
5. CD_MAVEN_DISABLE_CLI=false behavior: WhenDisableMvnCliEnvVarIsFalse_UsesMvnCliNormally_Async and related tests — verify that setting the env var to false or an invalid value still allows CLI to run normally.

These scenarios represent core functionality of the merged detector logic and should have corresponding tests.

The test uses the hardcoded string "CD_MAVEN_DISABLE_CLI" instead of the constant MvnCliComponentDetector.DisableMvnCliEnvVar. While the constant is internal and not directly accessible from the test assembly, the old MavenWithFallbackDetectorTests.cs used MavenWithFallbackDetector.DisableMvnCliEnvVar because that constant was also internal but accessible because tests were aware of it. Consider making DisableMvnCliEnvVar accessible via an [assembly: InternalsVisibleTo("Microsoft.ComponentDetection.Detectors.Tests")] attribute (as done in other parts of the codebase, e.g., src/Microsoft.ComponentDetection.Detectors/pip/IPyPiClient.cs:21) or changing the visibility to public, so the test references the constant directly instead of a magic string that can silently diverge.

The IFileUtilityService dependency is injected and assigned to this.fileUtilityService in the constructor, but it is never actually used anywhere else in MvnCliComponentDetector. The old MavenWithFallbackDetector used it to read the generated deps file content after a successful Maven CLI run (fileUtilityService.Exists(depsFilePath) and fileUtilityService.ReadAllText(depsFilePath)), but the new implementation instead relies on ComponentStreamEnumerableFactory.GetComponentStreams() to find all generated dependency files in the source directory. This unused dependency should be removed from the constructor signature and class to avoid confusion.

This test is copied from the now-deleted MavenWithFallbackDetectorTests but is no longer testing the correct behavior. In the old MavenWithFallbackDetector, the fallback to static parsing occurred when GenerateDependenciesFileAsync returned Success=true but fileUtilityService.ReadAllText(depsFilePath) returned an empty string. The new MvnCliComponentDetector does NOT use IFileUtilityService at all — it relies on ComponentStreamEnumerableFactory.GetComponentStreams() to find any generated bcde.mvndeps files. When CLI reports success but no bcde.mvndeps file exists in the test virtual filesystem, the new implementation does NOT fall back to static parsing (since no failedDirectories are recorded). As a result, this test likely passes with zero detected components, making the assertion detectedComponents.Should().ContainSingle() incorrect. The fileUtilityServiceMock setups are also dead code. This test should be removed or rewritten to reflect the actual behavior of the new implementation.

The updated comment at lines 23-27 is now grammatically incomplete. The sentence "This allows the MvnCliComponentDetector" ends without finishing the thought, then the next line "to safely share the same output file without race conditions." starts with "to" without a subject, making the full sentence read: "This allows the MvnCliComponentDetector to safely share the same output file without race conditions." This is actually a dangling incomplete sentence in the XML doc comment. The comment should be rephrased to form a complete, readable sentence, such as: "This allows the MvnCliComponentDetector to safely share the same output file without race conditions."

The expectedRemovedDetectors HashSet is defined independently in both CheckDetectorsRunTimesAndCounts (line 176) and ProcessDetectorVersions (line 247), with the same literal content { "MavenWithFallback" }. This duplication means any future updates to the list must be made in two places. Consider extracting it to a class-level field or property to avoid divergence.

The same expectedRemovedDetectors HashSet with identical content is also defined at line 176 in CheckDetectorsRunTimesAndCounts. Both definitions contain "MavenWithFallback" and would need to be updated together whenever the list changes.

The RunMavenCliDetectionAsync method in the updated MvnCliComponentDetector constructs the return value by concatenating two sources: (1) the results ConcurrentBag, which is populated with ProcessRequests for every successfully-processed deps file (line ~312), and (2) allGeneratedDependencyFiles which scans the entire source directory for ALL bcde.mvndeps files (lines 383-404). These two sources will both yield the same deps file for each successful Maven CLI run, causing ProcessMvnCliResult (and thus ParseDependenciesFile) to be called twice for the same file. This will double-count components in mvnCliComponentCount and may cause duplicate graph entries depending on the recorder's behavior. The allGeneratedDependencyFiles scan appears intended to handle sub-module deps files generated by Maven that were not explicitly tracked in results, but it should be filtered to exclude paths already present in results, or results should not add items that will also be picked up by allGeneratedDependencyFiles.

The new StaticParser_IgnoresDependenciesWithoutVersion_Async test only includes a single dependency without a version, asserting that zero components are detected. The equivalent test in the deleted MavenWithFallbackDetectorTests had two dependencies (one without version, one with a valid version), verifying that the valid one is still detected while the versionless one is skipped. This mixed case — where some dependencies have missing versions and others are valid — is now untested.

The expectedMatchCount calculation assumes exactly one log line per removed detector (oldMatches.Count - expectedRemovedDetectors.Count). However, each detector may have zero or more log entries (e.g., if MavenWithFallback ran but produced no output, or produced multiple entries). If MavenWithFallback contributed more than one match in the old log, this calculation will produce an over-permissive threshold (too few expected matches required), causing a legitimate lost-detector regression to be silently ignored.

Technically don't need this semaphore anymore, but i don't think there's any harm in keeping it there

There is an issue with adding this, as it would allow any detector to be removed after this and no longer fail this first check phase.

Since this is a test that runs as part of the PPE phase and not during the PR, i think it would be best to keep it as is and have that manual confirm step be the expectation.

We also just need to fix those PPE tests in general as they are flaky and pretty much always fail: https://dev.azure.com/cg-scus1/MyFirstProject/_build?definitionId=697&_a=summary

This one is fine though